KAFKA-20309: Limit SharePollEvent to single instance#21757
KAFKA-20309: Limit SharePollEvent to single instance#21757AndrewJSchofield merged 1 commit intoapache:trunkfrom
Conversation
There was a problem hiding this comment.
I would only add that we should add tests for the new event generation behaviour, mainly to catch regressions (e.g., no new event created on poll if there is an in-flight).
I wanted to link you the the similar async consumer test and turns out we're missing the coverage there too :) I will file a jira now to add that coverage to the async. For share, we can add it here or in that same jira (the test goal is the same, impl should be very similar I expect). As you prefer.
| shareMembershipManager.maybeReconcile(true)); | ||
|
|
||
| requestManagers.shareHeartbeatRequestManager.ifPresent(hrm -> { | ||
| ShareMembershipManager membershipManager = hrm.membershipManager(); |
There was a problem hiding this comment.
just for my understanding, is there a reason why we prefer to go through the HBReqMgr to get the shareMembershipManager here, instead of just calling onConsumerPoll on the shareMembershipManager we already had above?
There was a problem hiding this comment.
I just followed the pattern with the other group types. I don't think there was a reason this was different.
3 participants
When a share consumer is initially joining a group but not yet fetching
records, the poll loop has no records to wait for. Each iteration of the
loop was creating an instance of
SharePollEventwithout consideringwhether an in-flight event existed. As a result, many events could
briefly queue up for no good reason, only to be drained once the
consumer successfully joined the group.
The PR follows a similar design as used in the
AsyncKafkaConsumerformanaging its poll event, without that code's extra complication of
validating the position.
Reviewers: TaiJuWu tjwu1217@gmail.com, Lianet Magrans
lmagrans@confluent.io